Skip to content

http2: emit response event instead of trailers event #41405

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

http2: emit response event instead of trailers event #41405

wants to merge 1 commit into from

Conversation

MoonBall
Copy link
Member

@MoonBall MoonBall commented Jan 5, 2022

Fix #41251.

Should emit response event if cat == NGHTTP2_HCAT_HEADERS and status >= 200.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http2
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. labels Jan 5, 2022
@MoonBall MoonBall force-pushed the fix-http2-response branch from dc2be07 to 210ba97 Compare January 5, 2022 15:09
@MoonBall MoonBall force-pushed the fix-http2-response branch from 210ba97 to 38c46b1 Compare January 5, 2022 15:55
apapirovski
apapirovski approved these changes Jan 5, 2022
View reviewed changes
Copy link
Contributor

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice find. And actually makes sense after reading the nghttp2 docs https://nghttp2.org/documentation/enums.html#c.nghttp2_headers_category

@szmarczak
Copy link
Member

The naming is very confusing. HTTP/2 states that those are trailers (since HEADERS are sent with an END_STREAM flag), nghttp2 defines NGHTTP2_HCAT_RESPONSE (informational) + NGHTTP2_HCAT_HEADERS (final) and Node.js would emit response (however it also has a trailers event).

mcollina
mcollina approved these changes Jan 5, 2022
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina
Copy link
Member

The naming is very confusing. HTTP/2 states that those are trailers (since HEADERS are sent with an END_STREAM flag), nghttp2 defines NGHTTP2_HCAT_RESPONSE (informational) + NGHTTP2_HCAT_HEADERS (final) and Node.js would emit response (however it also has a trailers event).

Is this an objection? How would you like this PR to be changed?

@szmarczak
Copy link
Member

In the Examples section of RFC7540:

     HTTP/1.1 100 Continue            HEADERS
     Extension-Field: bar       ==>     - END_STREAM
                                        + END_HEADERS
                                          :status = 100
                                          extension-field = bar

     HTTP/1.1 200 OK                  HEADERS
     Content-Type: image/jpeg   ==>     - END_STREAM
     Transfer-Encoding: chunked         + END_HEADERS
     Trailer: Foo                         :status = 200
                                          content-length = 123
     123                                  content-type = image/jpeg
     {binary data}                        trailer = Foo
     0
     Foo: bar                         DATA
                                        - END_STREAM
                                      {binary data}

                                      HEADERS
                                        + END_STREAM
                                        + END_HEADERS
                                          foo = bar

in this instance there are three HEADERS frames. I think it would make sense to have an information event, like in HTTP/1 API. What do you think?

@aduh95
Copy link
Contributor

aduh95 commented Mar 12, 2022

This needs a rebase.

@aduh95
Copy link
Contributor

aduh95 commented Sep 18, 2023

This still needs a rebase :)

@aduh95
Copy link
Contributor

aduh95 commented May 11, 2024

Superseded by #41739

@aduh95 aduh95 closed this May 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

+1 more reviewer

@apapirovski apapirovski apapirovski approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

http2: trailers or response to be emitted?

7 participants

@MoonBall @nodejs-github-bot @szmarczak @mcollina @aduh95 @apapirovski @jasnell